fix: punish invalid dstx messages#7347
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds Sequence DiagramsequenceDiagram
participant Peer as P2P Peer
participant ProcessMsg as PeerManagerImpl::ProcessMessage
participant ValidateDSTX as ValidateDSTX
participant Misbehave as Misbehaving
Peer->>ProcessMsg: recv "dstx" message
ProcessMsg->>ValidateDSTX: validate DSTX
alt Unknown masternode (score=1, do_return=true)
ValidateDSTX-->>ProcessMsg: {1, true}
ProcessMsg->>Misbehave: Misbehaving(pfrom, 1, "invalid dstx")
Misbehave-->>ProcessMsg: penalty recorded
else Invalid DSTX structure (score=10, do_return=true)
ValidateDSTX-->>ProcessMsg: {10, true}
ProcessMsg->>Misbehave: Misbehaving(pfrom, 10, "invalid dstx")
Misbehave-->>ProcessMsg: penalty recorded
else Already have (score=0, do_return=true)
ValidateDSTX-->>ProcessMsg: {0, true}
ProcessMsg-->>Peer: return without penalty
else Valid (score=0, do_return=false)
ValidateDSTX-->>ProcessMsg: {0, false}
ProcessMsg->>ProcessMsg: continue to mempool acceptance
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 7b8a80f) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4f814a849
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!bRet) { | ||
| Misbehaving(pfrom.GetId(), 10, "invalid dstx"); |
There was a problem hiding this comment.
Avoid penalizing unverifiable DSTX relays
When this node is out of IBD but its active tip does not yet include the masternode that signed the DSTX, ValidateDSTX searches only the current tip and 23 ancestors and returns {false, true} from the “Can't find masternode” path. This new !bRet branch therefore increments the peer's misbehavior score for a DSTX that may be valid from the relaying peer's newer chain view, turning a local inability to verify into punishment; consider distinguishing unverifiable/unknown-masternode cases from structural or signature failures before calling Misbehaving.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR correctly applies a small misbehavior score (10) only on the invalid/error early-return paths of ValidateDSTX, leaving benign duplicate and mixing-rate-limited cases untouched. The new functional test exercises the unknown-masternode error path with a matching Python serializer for the dstx message. Two non-blocking observations: the !dmn path can rarely punish honest peers whose MN list is briefly stale, and only one of the three error paths is covered by the test.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/net_processing.cpp`:
- [SUGGESTION] src/net_processing.cpp:3584-3587: `!dmn` path can punish honest peers when local MN list is briefly stale
This change routes all three `{false, true}` returns from ValidateDSTX through `Misbehaving(..., 10, "invalid dstx")`, including the `!dmn` branch at lines 3584-3587. The 24-block lookback at lines 3565-3582 already mitigates short staleness, but an honest relayer can still trip this case during reorgs, slow validation, or restart catch-up shortly after IBD when the receiving node's deterministic MN list is more than 24 blocks behind the broadcaster's registration. Score 10 keeps the impact bounded (discouragement threshold is 100), but a well-connected peer that forwards multiple valid DSTXes against a stale node could still accumulate score over time. Unlike `!IsValidStructure()` and bad-signature, which are unambiguous protocol violations, missing-MN can be a benign timing artifact. Consider treating `!dmn` like the `!IsValidForMixingTxes` branch (`{true, true}`), or gating the punishment with `m_mn_sync.IsBlockchainSynced()`, so only structural and signature failures are punished.
|
Addressed the automated review feedback in Validation:
@coderabbitai review |
|
✅ Action performedReview finished.
|
I think this would still allow an attacker to waste our CPU / potentially DoS us? no? IMO we should punish for unknown MNs, but make it a small punishment, so that someone would need to send like 50 or 100 invalid / bad mn dstx before we ban? |
Previously, DSTX messages whose masternode we couldn't find in the deterministic MN list were silently dropped with no peer penalty. This let a malicious peer flood us with unverifiable DSTXes indefinitely. Apply a small misbehavior score (1) on that path so 100 such messages from one peer cross the discouragement threshold, while keeping the existing stronger penalty (10) for clearly malformed DSTXes and bad-signature DSTXes. The ValidateDSTX return value is changed from a bool to the misbehavior score itself; the call site now applies that score directly. Extend p2p_dstx.py to cover all three observable cases: small penalty for unknown-MN, stronger penalty for malformed structure, and the cumulative discouragement of a peer flooding unknown-MN DSTXes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Agreed — ignoring unknown-MN DSTX entirely leaves a cheap CPU/validation spam lane. I pushed
Validation: The test now covers the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/net_processing.cpp (1)
3542-3545: ⚡ Quick winUse a strong result type for DSTX validation scores.
The scoring logic looks correct, but
std::pair<int, bool>keeps this security-sensitive contract positional; old-style returns like{false, true}would still compile as a zero-penalty early return. A small named result type makes future edits safer.Suggested refactor
-// Returned score is the misbehavior penalty to apply to the relaying peer; 0 means no penalty. -// do_return signals the caller to stop further processing of the DSTX. -std::pair<int /*misbehavior_score*/, bool /*do_return*/> static ValidateDSTX(CDeterministicMNManager& dmnman, CDSTXManager& dstxman, ChainstateManager& chainman, - CMasternodeMetaMan& mn_metaman, CTxMemPool& mempool, CCoinJoinBroadcastTx& dstx, uint256 hashTx) +enum class DSTXMisbehaviorScore : int { + NONE = 0, + UNKNOWN_MASTERNODE = 1, + INVALID = 10, +}; + +struct DSTXValidationResult { + DSTXMisbehaviorScore misbehavior_score{DSTXMisbehaviorScore::NONE}; + bool do_return{false}; +}; + +static DSTXValidationResult ValidateDSTX(CDeterministicMNManager& dmnman, CDSTXManager& dstxman, ChainstateManager& chainman, + CMasternodeMetaMan& mn_metaman, CTxMemPool& mempool, CCoinJoinBroadcastTx& dstx, uint256 hashTx) @@ - return {10, true}; + return {DSTXMisbehaviorScore::INVALID, true}; @@ - return {0, true}; // not an error + return {DSTXMisbehaviorScore::NONE, true}; // not an error @@ - return {1, true}; + return {DSTXMisbehaviorScore::UNKNOWN_MASTERNODE, true}; @@ - return {0, true}; + return {DSTXMisbehaviorScore::NONE, true}; @@ - return {10, true}; + return {DSTXMisbehaviorScore::INVALID, true}; @@ - return {0, false}; + return {DSTXMisbehaviorScore::NONE, false}; @@ - const auto& [misbehavior_score, bDoReturn] = ValidateDSTX(*m_dmnman, m_dstxman, m_chainman, m_mn_metaman, m_mempool, dstx, hashTx); - if (bDoReturn) { - if (misbehavior_score > 0) { - Misbehaving(pfrom.GetId(), misbehavior_score, "invalid dstx"); + const auto dstx_validation = ValidateDSTX(*m_dmnman, m_dstxman, m_chainman, m_mn_metaman, m_mempool, dstx, hashTx); + if (dstx_validation.do_return) { + if (dstx_validation.misbehavior_score != DSTXMisbehaviorScore::NONE) { + Misbehaving(pfrom.GetId(), static_cast<int>(dstx_validation.misbehavior_score), "invalid dstx"); } return; }Also applies to: 3551-3555, 3588-3591, 3594-3604, 3610-3610, 4675-4679
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/net_processing.cpp` around lines 3542 - 3545, Replace the std::pair<int, bool> return type with a named result type (such as a struct with clearly named fields like misbehavior_score and do_return) to make the security-sensitive DSTX validation contract explicit and type-safe. Update the ValidateDSTX function signature at line 3542-3545 to use this new named type, then update all return statements within ValidateDSTX (around lines 3551-3555, 3588-3591, 3594-3604, 3610-3610) to construct instances of the new type using named initialization instead of positional pair construction. Finally, update all call sites of ValidateDSTX (around line 4675-4679) to use named field access instead of .first and .second accessors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/net_processing.cpp`:
- Around line 3542-3545: Replace the std::pair<int, bool> return type with a
named result type (such as a struct with clearly named fields like
misbehavior_score and do_return) to make the security-sensitive DSTX validation
contract explicit and type-safe. Update the ValidateDSTX function signature at
line 3542-3545 to use this new named type, then update all return statements
within ValidateDSTX (around lines 3551-3555, 3588-3591, 3594-3604, 3610-3610) to
construct instances of the new type using named initialization instead of
positional pair construction. Finally, update all call sites of ValidateDSTX
(around line 4675-4679) to use named field access instead of .first and .second
accessors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 161d5d50-ef31-421a-990b-b31c105d4c7e
📒 Files selected for processing (2)
src/net_processing.cpptest/functional/p2p_dstx.py
Replaces the positional std::pair<int, bool> return with a small DSTXValidationResult struct and a DSTXValidationScore enum (NONE=0, UNKNOWN_MASTERNODE=1, INVALID=10). Behavior is unchanged; the call site reads named fields and casts the enum for Misbehaving. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed CodeRabbit’s named-result nitpick in |
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both agents independently found no in-scope defects in PR #7347 at a834648, and the prior review at da0ea55 also had no findings to carry forward. The PR cleanly converts DSTX validation to a graduated misbehavior score, applies a small +1 penalty for unverifiable (unknown-MN) DSTXes to discourage floods while tolerating transient MN-removal races, and adds a focused functional test covering all three outcomes including the cumulative discouragement path. CodeRabbit had zero inline findings.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (a834648..f32588a) is a pure readability refactor in src/net_processing.cpp: ValidateDSTX now returns a typed DSTXValidationResult struct with an enum class DSTXValidationScore replacing std::pair<int,bool>. Score values (NONE=0, UNKNOWN_MASTERNODE=1, INVALID=10) match the prior literals, and the caller correctly compares against NONE and casts to int for Misbehaving — behavior is identical to the prior commit. Prior review at a834648 found no issues and there are no carried-forward findings; cumulative current-head inspection (DSTX validation, peer misbehavior accounting, Python DSTX serialization, functional test coverage of unknown-MN / structurally invalid / discouragement-flood) also surfaced none.
Note: GitHub does not allow PastaClaw to approve their own PR, so this clean automated review is posted as a comment review.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest push (7b8a80f) is a CI retrigger with no code changes since f32588a, which had a clean prior review. No carried-forward findings; no new findings in the latest delta. Cumulative PR state (DSTX validation scoring with named result type, low-score penalty for unknown-masternode relays, and the focused functional test) remains clean. GitHub does not allow PastaClaw to approve its own PR, so this is posted as a clean comment review rather than an approval.
fix: punish invalid dstx messages
Issue being fixed or feature implemented
Invalid CoinJoin broadcast transaction (
dstx) messages can return early fromvalidation without applying the same peer-accounting consequence used by other
invalid P2P messages.
This hardens DSTX handling by making invalid validation results contribute a low
misbehavior score while preserving benign early returns for duplicate or
otherwise non-error DSTX cases.
What was done?
result.
cases.
dstxmessages.How Has This Been Tested?
Tested on macOS arm64.
git diff --checkpython3 -m py_compile test/functional/p2p_dstx.py test/functional/test_framework/messages.pyconfig.site,--without-gui,--disable-bench,and
--disable-fuzz-binarymake -j8 src/dashdtest/functional/p2p_dstx.py --tmpdir=/tmp/dash_func_dstxtest/functional/p2p_dstx.py --tmpdir=/tmp/dash_func_dstx_2Breaking Changes
None.
Checklist